Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the examples argument parser problem of not handling booleans #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrewcheelightstep
Copy link

Modified the argument parser for the --use_tls argument because it wasn't parsing booleans correctly.

@carlosalberto
Copy link
Contributor

Hey @andrewcheelightstep

Thanks for the PR (and sorry for the delayed review). The change looks right, and the only thing I wonder is the default value - before this change, the expected default value was to use TLS, while with this change the default would be to not use it.

Maybe we should rename it to no_tls, so by default we stil use TLS? Else, we can merge, I'd say.

@katiabazzi @frenchfrywpepper

@joeblubaugh
Copy link
Contributor

Agree with @carlos - it's just an example program, but --disable_tls seems like a better name for this behavior. Then we can set store_false and default to TLS

@andrewcheelightstep
Copy link
Author

I have no problems with this. Is this something we can merge soon?

frenchfrywpepper added a commit that referenced this pull request Sep 5, 2018
This was the default before my proto change. And I noticed this
open PR #43
which had some feedback about that.
@andrewhsu andrewhsu added the bug label Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants